Admin section: complete the sidebar, kill double-edits, real Doctor - #289
Merged
Conversation
…or to real data
Three duplications and a fake panel, all in the admin section:
- Branding and Feature Flags registered into the main app sidebar while
their pages live under /admin in AdminLayout — the app sidebar grew
orphan "Appearance"/"System" groups (one floating above Dashboard) and
the admin sidebar/index were missing both screens. They now register
into ADMIN_SIDEBAR; branding gets order=105 so groups read
Access → Appearance → System.
- Branding was editable in two places: its own page and the generic
Settings → Modules editor. register_module_settings() gains an
optional manage_url; a module that declares one renders as a
"Managed on its own page" link card in the generic editor instead of
a second, raw editor for the same fields. Branding declares it.
- The Branding page repeated its own title and description inside the
form card, 40px under the identical page header. The card now starts
at the first field.
- The Doctor page was hardcoded fiction (doctor-data.ts: a fake billing
module, migrations that never existed, "Vite :5173" when dev runs on
5050). It now runs the real diagnostics engine per request, reads the
boot-time migration state plus recent alembic revisions, and reports
live environment facts. The fake dev-server and env-vars panels are
gone; the stat row is errors/warnings/modules/health, all real.
Fixing Doctor surfaced a false SM003 against audit_log: the render-call
resolver skipped annotated assignments (NAME: Final = ...) and f-string
constants, so PAGE_BROWSE = f"{MODULE_NAME}/Browse" never resolved. The
resolver now handles both shapes and refuses genuinely dynamic values.
Polish alongside: Feature Flags' near-empty scope card collapses into
one toolbar row; Background Tasks table headers adopt the uppercase
style every other admin table uses; "1 fields set" in Audit Log gets
CLDR plural forms.
Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
Deploying simple-module-python with
|
| Latest commit: |
43ca612
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3baf036c.simple-module-python.pages.dev |
| Branch Preview URL: | https://worktree-admin-redesign.simple-module-python.pages.dev |
The Admin Panel shell used red everywhere — sidebar tint, badge, logo mark, avatar, active states — which read as a warning, not a place. The admin area now speaks the same visual language as the app sidebar: primary-accent active states, the branding-driven logo mark, and an emerald Admin Panel badge. The badge and the panel's own menu are the wayfinding; a color no longer shouts it. Because the accent rides the branding primary tokens, a deployment's custom brand color now restyles the admin area along with the app. The red-tinted --color-admin-* tokens had no other consumers, so they go too. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
Review side-findings against the merged landing-page commit: the clipboard promise could setState after unmount on an SPA nav, and modules/keycloak inherited a test-exclude glob that TS anchors at the declaring file, so a future keycloak test file would fail typecheck. Verified: CopyCommand tests 5/5, tsc clean on both projects. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
In scope for this branch:
- settings module API: PUT/DELETE now 409 for any package declaring
manage_url. The double-edit invariant was only enforced in the Inertia
UI; a direct JSON call still wrote Branding's fields behind its
dedicated page. Branding's own editor uses /api/branding/*, so it is
unaffected. Regression tests added.
- doctor.py no longer keeps its own hardcoded "host/alembic.ini" copy —
migrations.py gains a shared script_directory() helper that
resolve_head_revision, check_migrations and the Doctor page all use.
Review side-findings on pre-existing code, kept because they are small,
tested, and in the shell this branch already touches:
- auth_local: login_redirect_url falls back to /dashboard/ when the
setting is blank, instead of handing the frontend "" (router.visit("")
is a no-op reload of /login).
- CommandPalette: hardcoded "Navigation"/"Account" group headings now go
through t(keys.ui.nav_groups.*) per CLAUDE.md; the duplicated
post-method check is extracted as isPostMenuItem() and shared with
SidebarUserMenu.
Verified: make lint clean, 2160 python + 127 js tests pass.
Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
- login_redirect_url: normalise blank to the default on UsersSettings
itself rather than at one call site. The previous pass guarded only the
local-login view; Keycloak and generic OAuth read the same setting
straight into a Location header, so a blanked value still produced an
empty redirect there. Hydration and apply_changes_and_reload both
reconstruct through the class, so all three consumers are covered.
- Doctor diagnostics list: include `file` in the React key. One module can
raise the same code with the same file-agnostic message for two
different files (two SM022 hits), and the rows collided on key, so React
silently dropped one real finding.
- SM003 f-string resolution now runs to a fixed point, so a chained
constant (PREFIX = f"{NAME}/sub"; PAGE = f"{PREFIX}/Browse") resolves
instead of reporting the page as an orphan.
Reviewed and not changed: the sparse module badges on the migrations
panel are correct — alembic only sets branch_labels on a branch's
founding revision, which is the per-module convention CLAUDE.md
documents. The duplicated table-header class literal predates this
branch in four other pages; deduping it belongs with that cleanup, not
here. Deleting the plain `fields_set` catalog key would break its call
site: it is the anchor keys.generated.ts emits, and i18next appends
_one/_other at lookup time.
Verified: make lint clean, 2164 python + 127 js tests pass.
Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
The previous pass fixed only UsersSettings. Keycloak has its own settings class with its own login_redirect_url, and its callback reads that copy into a Location header — so blanking it there still produced an empty redirect, and the last commit message overclaimed by saying Keycloak was covered. The shared normalisation lives in simple_module_core.redirect_safety (already the home for safe_next/safe_next_or_none and imported by both providers) rather than in one module, since the two provider modules must not import each other. Each provider keeps its own field validator, which is where hydration and apply_changes_and_reload both run. Verified: make lint clean, 2167 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
… theme - /admin/doctor did blocking filesystem walks and AST parses (module coupling/page checks, the alembic script directory) inline on the event loop, serially after the DB fetch. They now run via asyncio.to_thread alongside it, so one doctor request no longer stalls every other coroutine on the worker for a full framework scan. Still live data — no caching added. - AdminLayout's THEME was byte-for-byte AuthenticatedLayout's after this branch dropped the red admin skin, which is exactly the drift that commit set out to remove. Both now spread DEFAULT_SIDEBAR_THEME and override only mobileTitleLabel. The theme moved to its own layouts/sidebar-theme.ts (SidebarLayout was over the 300-line cap) and is re-exported from SidebarLayout so existing importers are unaffected. Verified: make lint clean, 2167 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
…toggles Browser QA of the admin area found two defects worth fixing here: - Module settings masked `reset_password_token_lifetime_seconds` as a secret, so an admin could neither read nor edit a plain duration. The name pattern already dodges the bare words "token" and "key", but it cannot dodge "password" — and that field is an int. Credential material is always a string, so the declared type now gates the match, which closes the whole class rather than this one name. The mask-sentinel stripper is unaffected: it already requires the value to equal the string sentinel. - The sidebar's icon-only open/close buttons showed no focus ring under keyboard navigation (WCAG 2.4.7). Button's default ring-ring/50 is invisible on the near-black sidebar, so those two get a light ring. Text links beside them already read via the UA outline. Not changed, with reasons: the Users "You're the only account" prompt renders above a one-row table by design — it is an onboarding nudge, not an empty state, and UsersEmpty.tsx documents why hiding the row would be wrong. Branding's silent handling of server 422s, the uncounted maxlength truncation, and BackgroundTasks accepting a negative max_retries are all real but pre-existing, outside this branch's changes, and want their own change with a validation design behind it. Verified: make lint clean, 2169 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
Keyboard QA kept landing on a full-screen element with no visible focus ring. It was the mobile overlay backdrop, which shares the "Close sidebar" label with the real X button and, being full-screen, can't show a meaningful ring. It is the click-outside affordance; the X button beside the logo is the keyboard route and already carries a ring. So the backdrop leaves the tab order and the mouse path is untouched. Verified in the browser: Tab reaches the X button with a visible ring and never lands on the backdrop; clicking the backdrop still closes. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
Round-2 review flagged a latent trap in the type gate: value_type_for_field reports "json" for any union, so a secret declared `str | None` is not the "string" case and would have been silently un-masked. No field hits this today, but the failure direction was wrong. Inverted the test — exempt the types that cannot hold a credential (int/float/bool) instead of admitting only strings — so an unexpected type stays masked. Same fix for the original bug, safe when it is wrong. Verified: make lint clean, 2170 python + 127 js tests pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
Each of these is easy to regress silently and cheap to assert, and three of them already regressed once during this branch's own review: - the app sidebar delegates to a single Administration link, and the admin shell reaches all seven screens (Branding and Feature Flags used to register into the app sidebar while rendering in the admin layout); - Branding is linked to, never re-edited, in the generic module editor — asserted in the UI and against the JSON API, since guarding only the screen leaves the invariant one fetch away from being bypassed; - Doctor reports live environment and diagnostics, and no longer renders the retired demo fixtures (a billing module that never existed, a dev-server panel with the wrong port). Verified: 5/5 new specs pass, full e2e suite 30/30 green. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
antosubash
marked this pull request as ready for review
August 28, 2026 09:59
CI's e2e job runs a module subset that excludes Branding, so the new admin specs failed there on a deployment choice rather than a defect — the hardcoded seven-screen roster is the bug in a framework whose whole point is that modules are pluggable. The shell test now checks each admin link that IS present against its canonical URL and requires only the framework's own screens, so a module vanishing from the sidebar still fails while a trimmed install does not. The two Branding-specific specs skip when the module is absent, waiting for a known-present link first — count() does not auto-wait, so checking it against a client-rendered sidebar would skip on a slow render instead of a real absence. Branding is also added to the e2e job's module list: the "edited on its own page, never in the generic editor" rule has a server-side 409 half, and a security-relevant invariant deserves CI coverage rather than a skip. Verified both ways: 5/5 pass with Branding installed, 3 passed + 2 skipped without it, and `make lint` is clean. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
CI failed both Python tests and typecheck on a file this branch never touched. The cause is that typer's layout is not fixed by its version number: this machine's typer 0.27.1 ships `typer._click.exceptions` and no `typer.exceptions`, while CI's 0.27.1 ships the reverse. The test named only the private `typer._click` path, so where that import failed the raised Exit was not in the expected tuple and propagated uncaught — passing locally and failing in CI on the same lockfile. Probe both module paths and keep whichever resolves, rather than asserting a layout. `click.exceptions.Exit` stays in the tuple as the floor, so the test still fails if nothing raises at all. Verified: 8/8 in that file, make lint clean, 2170 python + 127 js pass. Claude-Session: https://claude.ai/code/session_01KsFU6aApjwjBDWc51AXPui
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes every duplication in the admin section and polishes it, building on the
/adminrestructure from #274.Dedupe
/admininAdminLayoutbut registered their menu entries into the main app sidebar — leaving an orphan "Appearance" group floating above Dashboard, a second "System" group, and an admin sidebar missing two of its seven screens. The app sidebar is now Dashboard / Content / Administration; the admin sidebar reads Access → Appearance → System, complete.register_module_settings()gained an optionalmanage_url; a module that declares one appears in Settings → Modules as a "Managed on its own page →" link card instead of a second raw editor over the same fields. The generic JSON API enforces the same invariant with a 409, so the rule cannot be bypassed with a directPUT.Real Doctor
The Doctor page's checks, migrations, and dev-server panels were hardcoded demo data (a
billingmodule that never existed, "Vite :5173" when dev runs on 5050). It now runs the real diagnostics engine per request — off the event loop, since those checks walk and parse the whole codebase — plus real migration state and live environment facts.Wiring it surfaced a false SM003 the framework printed on every dev boot: the render-call resolver couldn't read
PAGE: Final = f"{MODULE_NAME}/Browse". Fixed at the source, with regression tests, including chained constants.Admin shell recolored
The Admin Panel dropped its alarm-red skin for the app's primary accent. Because the accent rides the branding tokens, a deployment's custom brand colour now restyles the admin area too.
Fixes found by review and QA
Review ran six passes and browser QA three iterations; between them they caught 20 defects, two of which this branch had introduced:
PUT/DELETEstill wrote Branding's fields behind its own page.login_redirect_urlproduced an empty navigation target. Normalisation now lives on the settings classes (both auth providers, via a shared helper in the framework) rather than at one call site.reset_password_token_lifetime_seconds— anint— was masked as a secret because its name contains "password", so admins could neither read nor edit it. Masking is now gated on the declared type, and fails safe on types it doesn't recognise.CopyCommandunmount guard and akeycloaktsconfig test-exclude, found while reviewing the merged base.Verification
Deliberately left alone
Investigated and judged out of scope rather than skipped: the Users "You're the only account" prompt (an onboarding nudge that intentionally sits above a valid one-row table), Branding's silent handling of server 422s, and a table-header class repeated across five pages (this branch added the fifth; the other four predate it).
Test plan
/admin/and confirms the sidebar reads Access → Appearance → System with all seven screens/admin/doctor/shows live diagnostics and real migration hashes